Fix random "ERROR_FILE_NOT_FOUND" when unmounting with absolute path#40092
Fix random "ERROR_FILE_NOT_FOUND" when unmounting with absolute path#40092chemwolf6922 wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent ERROR_FILE_NOT_FOUND failures when unmounting a VHD by an absolute path without the \\?\ prefix, by retrying the detach using a normalized path representation.
Changes:
- Retry
DetachDiskwith a normalized/full path onERROR_FILE_NOT_FOUND(not only for relative paths), to handle\\?\prefix mismatches. - Update the inline comment to reflect the broadened retry behavior.
Comments suppressed due to low confidence (1)
src/windows/common/WslClient.cpp:1226
- The unconditional retry normalizes the disk argument via filesystem::GetFullPath(), which only works for real filesystem paths. If the user passes a non-filesystem disk identifier (e.g. passthrough paths like "\.\PHYSICALDRIVE…") and DetachDisk returns ERROR_FILE_NOT_FOUND, GetFullPath will throw (often ERROR_INVALID_NAME), changing the user-visible failure from ERROR_FILE_NOT_FOUND to a different error. Consider making normalization best-effort (catch/ignore GetFullPath failures and fall back to the original HRESULT), or gate the retry to VHD/filesystem-style paths only.
// Retry with the normalized path to handle relative paths and \\?\ prefix mismatches.
if (result == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
{
// retry dismounting with the absolute path
const auto absoluteDisk = wsl::windows::common::filesystem::GetFullPath(filesystem::UnquotePath(disk).c_str());
value = service.DetachDisk(absoluteDisk.c_str());
|
Hey @chemwolf6922 👋 — Following up on this PR. CI is all green and it's a nice targeted fix. There's 1 unresolved review question from @benhillis asking why the bug is non-deterministic — could you provide a response when you get a chance? That should help move this toward approval. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/windows/common/WslClient.cpp:1226
- This retry normalizes the path via
filesystem::GetFullPath(), which requires opening the path and will throw if the VHD path can’t be accessed (missing, ACL changed, etc.). Since this codepath is intended to recover from a registry path-string mismatch (\\?\\vs non-prefixed), consider normalizing without requiring the file to exist (e.g., compute an absolute path with Win32 path APIs and/or try both a non-prefixed and a\\?\\-prefixed variant) or at least guard the retry so normalization failures don’t replace the original detach failure semantics.
// Retry with the normalized path to handle relative paths and \\?\ prefix mismatches.
if (result == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
{
// retry dismounting with the absolute path
const auto absoluteDisk = wsl::windows::common::filesystem::GetFullPath(filesystem::UnquotePath(disk).c_str());
value = service.DetachDisk(absoluteDisk.c_str());
|
Hi, @benhillis , the problem was root caused and I added the test for it. Could you please take another look? Thanks. |
benhillis
left a comment
There was a problem hiding this comment.
Ok this makes sense now, thanks for fixing!
Summary of the Pull Request
Fix this random issue where sometimes unmount with the absolute path results in ERROR_FILE_NOT_FOUND
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
This PR adds the MountTests::MountTests::AbsolutePathVhdUnmountAfterVMTimeout test case. This test case waits for the VM to timeout before unmounting the vhd. Which will trigger the original issue w/o this patch.